gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREF#134377
gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREF#134377corona10 merged 21 commits intopython:mainfrom
Conversation
Objects/obmalloc.c
Outdated
There was a problem hiding this comment.
The default build implementation in pycore_pymem.h doesn't include the Py_NewRef().
My preference is to keep the Py_NewRef(value) in the callers, i.e.:
_PyObject_XSetRefDelayed(dictptr, Py_NewRef(value));to match the semantics of Py_XSETREF.
|
Interesting... CI passed in my local but not in the CI.. :( |
|
Ah okay.. |
corona10
changed the title
|
Why do we need Also, what is "delayed"? Either choose a clearer name, or add an explanatory comment in the header file. |
corona10
changed the title
IIUC, the critical section is for the root object to prevent other threads from mutating the child object, and a delayed reference to prevent use-after-free from other threads that reference the child object. |
|
Gentle ping @kumaraditya303 @vstinner |
Objects/genobject.c
Outdated
There was a problem hiding this comment.
Maybe add gh-133931: prefix to these comments.
Include/internal/pycore_pymem.h
Outdated
There was a problem hiding this comment.
Why not adding this function to pycore_object.h header instead?
There was a problem hiding this comment.
Because _PyObject_XDecRefDelayed is also declared here?
There was a problem hiding this comment.
I think it's fine to place _PyObject_XDecRefDelayed and _PyObject_XSetRefDelayed in pycore_object.h instead if you prefer. (I agree that they should probably be in the same header as each other).
Objects/typeobject.c
Outdated
There was a problem hiding this comment.
Is it correct issue reference for __dict__ too?
colesbury
left a comment
colesbury
left a comment
There was a problem hiding this comment.
Looks good - a few comments below
Include/internal/pycore_pymem.h
Outdated
There was a problem hiding this comment.
I think it's fine to place _PyObject_XDecRefDelayed and _PyObject_XSetRefDelayed in pycore_object.h instead if you prefer. (I agree that they should probably be in the same header as each other).
6 participants
gen_set_nameandgen_set_qualnamethread-safe in free-threaded builds #133931